Proofreading#669
Conversation
m-philipps
commented
May 13, 2026
m-philipps
commented
- Clarified the mapping table
- Minor fixes to links, spelling or improving readability (subjectively)
- Suggested an alternative version of Daniel's Figure that is read from left to right instead of clock-wise. Feel free to reject.
|
|
||
| A valid PEtab identifier (see :ref:`v2_identifiers`) that is not defined in | ||
| any other part of the PEtab problem. | ||
| A valid PEtab identifier (see :ref:`v2_identifiers`) that is globally unique. |
There was a problem hiding this comment.
This was replaced with "globally unique", because the documentation does not define "define" and the original description would make me think that the petabEntityId can not be a parameterId.
There was a problem hiding this comment.
because the documentation does not define "define"
I see that point, but I don't think this change resolves it. If it isn't used in any place other than the mapping table, the petabEntityId is rather useless. If it is used somewhere else, one still needs to distinguish between definition and reference.
the original description would make me think that the
petabEntityIdcan not be aparameterId.
So far, this was the assumption because we didn't consider any model format where model parameters could have names that aren't compatible with PEtab IDs.
To support that case, the parameter table description would have to be updated as well.
This would be a format change then and should go to a different PR.
| This identifier may be referenced in condition, measurement, parameter and | ||
| observable tables, but cannot be referenced in the model itself. | ||
|
|
||
| The ``petabEntityId`` may be the same as the ``modelEntityId``, but it must |
There was a problem hiding this comment.
Removed because under this restriction, the ids can never be the same. Also, with allowing the modelEntityId column to be empty, the use case of using the mapping table for further, optional columns is already covered.
There was a problem hiding this comment.
the ids can never be the same
But that's explicitly allowed here, isn't it? I don't consider an identity mapping an alias.
The main point here was to prevent recursive mappings such as A => B; B => C; C => D; ... to keep the things simpler. I think this should be preserved. If the goal is changing that, please move it to a dedicated PR.
dweindl
left a comment
dweindl
left a comment
There was a problem hiding this comment.
Thanks, the typesetting changes look good to me. Please move the suggested PEtab changes to dedicated PRs for further discussion.
There was a problem hiding this comment.
I prefer the old clockwise ordering, but okay to change it if the majority prefers left-to-right.
Maybe put that into a separate PR.
|
|
||
| A valid PEtab identifier (see :ref:`v2_identifiers`) that is not defined in | ||
| any other part of the PEtab problem. | ||
| A valid PEtab identifier (see :ref:`v2_identifiers`) that is globally unique. |
There was a problem hiding this comment.
because the documentation does not define "define"
I see that point, but I don't think this change resolves it. If it isn't used in any place other than the mapping table, the petabEntityId is rather useless. If it is used somewhere else, one still needs to distinguish between definition and reference.
the original description would make me think that the
petabEntityIdcan not be aparameterId.
So far, this was the assumption because we didn't consider any model format where model parameters could have names that aren't compatible with PEtab IDs.
To support that case, the parameter table description would have to be updated as well.
This would be a format change then and should go to a different PR.
| This identifier may be referenced in condition, measurement, parameter and | ||
| observable tables, but cannot be referenced in the model itself. | ||
|
|
||
| The ``petabEntityId`` may be the same as the ``modelEntityId``, but it must |
There was a problem hiding this comment.
the ids can never be the same
But that's explicitly allowed here, isn't it? I don't consider an identity mapping an alias.
The main point here was to prevent recursive mappings such as A => B; B => C; C => D; ... to keep the things simpler. I think this should be preserved. If the goal is changing that, please move it to a dedicated PR.
2 participants
